Skip to content

Conversation

@DongChyeon
Copy link
Member

@DongChyeon DongChyeon commented Sep 8, 2025

Related issue 🛠

closed #248

어떤 변경사항이 있었나요?

  • 🐞 BugFix Something isn't working
  • 🎨 Design Markup & styling
  • 📃 Docs Documentation writing and editing (README.md, etc.)
  • ✨ Feature Feature
  • 🔨 Refactor Code refactoring
  • ⚙️ Setting Development environment setup
  • ✅ Test Test related (Junit, etc.)

CheckPoint ✅

PR이 다음 요구 사항을 충족하는지 확인하세요.

  • PR 컨벤션에 맞게 작성했습니다. (필수)
  • merge할 브랜치의 위치를 확인해 주세요(main❌/develop⭕) (필수)
  • Approve된 PR은 assigner가 머지하고, 수정 요청이 온 경우 수정 후 다시 push를 합니다. (필수)
  • BugFix의 경우, 버그의 원인을 파악하였습니다. (선택)

Work Description ✏️

  • collect {} → first() 로 변경하여 DB 변경 발생 시 알람이 계속 재스케줄링되는 문제를 해결
  • 스케줄 호출 지점을 추적하기 위해 로깅 추가 (ScheduleTrace 로그와 예약 시간 출력)
  • 비반복 알람이 해제된 뒤에도 다음날 재등록되는 현상 방지

Uncompleted Tasks 😅

N/A

To Reviewers 📢

Summary by CodeRabbit

  • Bug Fixes

    • 재부팅/재예약 시 비활성 알람이 예약되던 문제를 수정하여 활성 알람만 재예약됩니다.
    • 중복 예약으로 인한 예기치 않은 다중 알림 발생 가능성을 줄였습니다.
  • Refactor

    • 재예약 로직을 간소화해 불필요한 반복 처리와 배터리 소모를 감소시키고 안정성을 높였습니다.
  • Chores

    • 스케줄링 과정의 내부 로깅을 추가해 알람 동작에 대한 가시성과 장애 진단을 개선했습니다.

@coderabbitai
Copy link

coderabbitai bot commented Sep 8, 2025

Walkthrough

AndroidAlarmScheduler에 내부 로그 유틸과 호출을 추가했다. RescheduleAlarmReceiver는 알람 플로우에서 first()로 단일 스냅샷을 가져와 isAlarmActive가 true인 알람만 재등록하도록 변경했다. 공개 API 시그니처 변경은 없다.

Changes

Cohort / File(s) Change Summary
로깅 유틸 추가
core/alarm/src/main/java/com/yapp/alarm/AndroidAlarmScheduler.kt
android.util.Log 임포트 및 private logSchedule(...) 추가. 반복/비반복/주간 재예약 시 스케줄링 직전 로그 호출 삽입. 기능 로직과 공개 시그니처 변경 없음.
재예약 흐름 수정
core/alarm/src/main/java/com/yapp/alarm/receivers/RescheduleAlarmReceiver.kt
Flow 수집을 first()로 단일 리스트 스냅샷 획득으로 변경. isAlarmActive == true인 알람만 필터링하여 스케줄. kotlinx.coroutines.flow.first 임포트 추가.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant SYS as System
  participant RCV as RescheduleAlarmReceiver
  participant REPO as AlarmRepository (Flow)
  participant SCH as AndroidAlarmScheduler

  SYS->>RCV: BOOT_COMPLETED/시간대 변경 등 브로드캐스트
  RCV->>REPO: alarmsFlow.first()
  REPO-->>RCV: List<Alarm>
  RCV->>RCV: active = alarms.filter { isAlarmActive }
  alt active 비어있지 않음
    loop 각 active 알람
      RCV->>SCH: schedule(alarm)
      SCH-->>RCV: 완료
    end
  else 없음
    RCV-->>SYS: 처리 종료
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Assessment against linked issues

Objective Addressed Explanation
비반복 알람이 해제 후에도 다음날 예약되는 문제 해결 (#248)

Assessment against linked issues: Out-of-scope changes

Code Change Explanation
내부 로깅 헬퍼 및 로그 호출 추가 (core/alarm/src/main/java/com/yapp/alarm/AndroidAlarmScheduler.kt: 다수 위치) 버그 원인 해결과 직접 관련 없는 진단용 변경으로 보이며, 링크된 이슈의 범위를 벗어남 여부가 명확히 정의되지 않음. 그러나 기능 동작에는 영향 없음.
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix/#248-alarm-one-time-reschedule

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@DongChyeon
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Sep 8, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
core/alarm/src/main/java/com/yapp/alarm/receivers/RescheduleAlarmReceiver.kt (1)

24-31: BroadcastReceiver 비동기 처리: goAsync() 없이 launch 사용은 작업 손실 위험이 큽니다

onReceive가 반환되면 프로세스가 정리될 수 있어 예약 작업이 중단될 수 있습니다. BOOT_COMPLETED 처리에서는 goAsync()로 수명을 연장하고 finish()를 보장하세요. 또한 내부에서 다시 CoroutineScope를 만들지 말고 suspend 함수로 분리해 호출하는 편이 안전합니다.

적용 예시(diff):

@@
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.launch
+import android.util.Log
@@
   override fun onReceive(context: Context?, intent: Intent?) {
@@
-    if (intent.action == Intent.ACTION_BOOT_COMPLETED) {
-      rescheduleAlarm()
-    }
+    if (intent.action == Intent.ACTION_BOOT_COMPLETED) {
+      val pending = goAsync()
+      CoroutineScope(SupervisorJob() + Dispatchers.IO).launch {
+        try {
+          rescheduleAlarm()
+        } catch (t: Throwable) {
+          Log.e("RescheduleAlarmReceiver", "Failed to reschedule alarms", t)
+        } finally {
+          pending.finish()
+        }
+      }
+    }
@@
-  private fun rescheduleAlarm() {
-    CoroutineScope(Dispatchers.IO).launch {
-      val alarms = alarmUseCase.getAllAlarms().first()
-      alarms
-        .filter { it.isAlarmActive }
-        .forEach { alarm -> androidAlarmScheduler.scheduleAlarm(alarm) }
-    }
-  }
+  private suspend fun rescheduleAlarm() {
+    val alarms = alarmUseCase.getAllAlarms().first()
+    alarms
+      .filter { it.isAlarmActive }
+      .forEach { alarm -> androidAlarmScheduler.scheduleAlarm(alarm) }
+  }

Also applies to: 33-40

🧹 Nitpick comments (2)
core/alarm/src/main/java/com/yapp/alarm/receivers/RescheduleAlarmReceiver.kt (1)

28-31: 업데이트/시간 변경 대비 추가 액션 고려

앱 업데이트(ACTION_MY_PACKAGE_REPLACED)나 시간/타임존 변경(ACTION_TIME_CHANGED, ACTION_TIMEZONE_CHANGED) 시에도 재예약이 필요하다면 동일 경로로 처리하도록 액션을 확장하는 것을 검토해 주세요. 매니페스트 권한/익스포트 설정도 함께 확인 필요합니다.

core/alarm/src/main/java/com/yapp/alarm/AndroidAlarmScheduler.kt (1)

20-27: 디버그 로그 가드 및 detekt 경고 해결 권장

매 호출 시 Throwable/Instant 생성은 비용이 큽니다. DEBUG 수준에서만 로그하도록 가드하고, detekt(ThrowingExceptionsWithoutMessageOrCause) 경고를 피하려면 Throwable에 메시지를 넣으세요.

적용 예시(diff):

   private fun logSchedule(tag: String, alarm: Alarm, triggerMillis: Long, extra: String = "") {
-    Log.d("ScheduleTrace", "scheduleAlarm Called", Throwable())
-    Log.d(
-      "AlarmSchedule",
-      "[$tag] id=${alarm.id}, repeatDays=${alarm.repeatDays}, " +
-          "time=${java.time.Instant.ofEpochMilli(triggerMillis)} $extra",
-    )
+    if (!Log.isLoggable("AlarmSchedule", Log.DEBUG)) return
+    Log.d("ScheduleTrace", "scheduleAlarm called", Throwable("call-site trace"))
+    Log.d(
+      "AlarmSchedule",
+      "[$tag] id=${alarm.id}, repeatDays=${alarm.repeatDays}, " +
+          "time=${java.time.Instant.ofEpochMilli(triggerMillis)} $extra",
+    )
   }

추가로 현지 디버깅 편의를 위해 LocalDateTime(기기 타임존)도 함께 출력하는 것(선택)을 고려해도 좋습니다.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7723cd and 57a42df.

📒 Files selected for processing (2)
  • core/alarm/src/main/java/com/yapp/alarm/AndroidAlarmScheduler.kt (5 hunks)
  • core/alarm/src/main/java/com/yapp/alarm/receivers/RescheduleAlarmReceiver.kt (2 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
core/alarm/src/main/java/com/yapp/alarm/AndroidAlarmScheduler.kt

[warning] 21-21: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.

(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Continuous Deployment
🔇 Additional comments (2)
core/alarm/src/main/java/com/yapp/alarm/receivers/RescheduleAlarmReceiver.kt (1)

35-38: first() + 활성 알람 필터링 적용은 문제의 핵심 원인을 잘 짚었습니다

DB 변경 스트림을 지속 수집하지 않고 단일 스냅샷만 사용하며 isAlarmActive만 재예약하는 전략은 “비반복 알람이 다음날 다시 예약” 문제를 효과적으로 차단합니다. 👍

core/alarm/src/main/java/com/yapp/alarm/AndroidAlarmScheduler.kt (1)

44-44: 스케줄링 직전 로그 주입은 트러블슈팅에 유용합니다

각 진입점(REPEAT/NON_REPEAT/RESCHEDULE_WEEKLY)에 동일 포맷으로 로그를 남겨 원인 추적이 수월해집니다. 유지하세요.

Also applies to: 55-55, 66-66

@DongChyeon DongChyeon merged commit 117f65d into release/1.1.3 Sep 8, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants